Skip to content

Conversation

@florianl
Copy link
Member

@florianl florianl commented Aug 6, 2025

Description

This is the next subtype as listed in #40489.

Link to tracking issue

Fixes #40163.

Testing

Documentation

This is the next subtype as listed in open-telemetry#40489.

Fixes open-telemetry#40163.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
@florianl
Copy link
Member Author

Friendly ping @TylerHelmuth, @evan-bradley and @edmocosta as the respective code owners.

@florianl
Copy link
Member Author

Friendly ping for feedback @mx-psi

@mx-psi
Copy link
Member

mx-psi commented Aug 18, 2025

@florianl Can you post in #otel-collector-dev (if you get no reply after a reasonable time feel free to ping the codeowners there as well)

return tCtx.GetProfileLocation().Address(), nil
},
Setter: func(_ context.Context, tCtx K, val any) error {
if v, ok := val.(uint64); ok {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use int64 for getting and setting integer values. OTTL cannot produce uint64 values unless it's coming from non-standardized paths, which should be avoided.

ctxprofilelocation.Name,
ctxprofilelocation.DocRef,
cacheGetter,
map[string]ottl.PathExpressionParser[TransformContext]{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we're missing handling the profilesample context (all places), which is a higher location's context.

// ....
// ┌──────────────────┐
// │      Profile     │
// └──────────────────┘
//   │                                n-1
//   │ 1-n         ┌───────────────────────────────────────┐
//   ▼             │                                       ▽
// ┌──────────────────┐   1-n   ┌──────────────┐      ┌──────────┐
// │      Sample      │ ──────▷ │   KeyValue   │      │   Link   │
// └──────────────────┘         └──────────────┘      └──────────┘
//   │                    1-n       △      △
//   │ 1-n        ┌─────────────────┘      │ 1-n
//   ▽            │                        │
// ┌──────────────────┐   n-1   ┌──────────────┐
// │     Location     │ ──────▷ │   Mapping    │
// └──────────────────┘         └──────────────┘
// ....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the diagram suggests a direct relation between message Sample and message Location this is actually not the case.

https://github.com/open-telemetry/opentelemetry-proto/blob/6836a416404eb8ff73636dd552dcbd59bd7a5a20/opentelemetry/proto/profiles/v1development/profiles.proto#L416-L434

message Sample holds indices to message Location, that are hold in ProfilesDictionary. Therefore there is no direct connection between Sample and Location.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If sample holds locations_start_index and locations_length, doesn't it mean it's a logical 1-N relation between both types, even though location is shared? If not, which type owns the sample? I got confused with https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/profiles/v1development/profiles.proto#L222-L230 and the diagram, and I'm trying to understand the access pattern here, for example, would we need to access any profilesample field from a profilelocation statement/condition? Like we currently do with spanevent and span: spanevent.attributes["foo"] == "bar" and span.kind != SPAN_KIND_UNSPECIFIED?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would we need to access any profilesample field from a profilelocation statement/condition?

This is hardly possible with reasonable resources. In theory it could be possible by following these steps:

  • Figure out the index of profileLocation in ProfilesDictionary
  • Walk all profileSample and check for every sample, if the location index is in the scope of locations_start_index to locations_length

One thing, that is no depicted in the shown diagram of the protocol, are the intermediate steps via ProfilesDictionary.
If you look at profileSample, it just holds indices that point to something in ProfilesDictionary.

Copy link
Contributor

@edmocosta edmocosta Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it doesn't make it different from other types then. Abstracting the proto details for a moment, if the logical hierarchy is profile -> sample -> location (indepently of where location is stored), then we're dealing with the same span and spanevent pattern, but with a deeper level.

For example, the transform processor currently supports the profile data, so it goes down into the pdata until it finds the profiles (see). Then it builds the transformation context for that type, including all needed data, and runs it.

Once we add support for the profile's sample, it will require another function, that does almost the same, but going a bit deeper, profiles -> sample.

Finally, to support locations, we would need to do exactly the same, but again, going a level deeper, and also including on the NewTransformContext all data dependencies, so all higher contexts data (up on the proto definition) can be accessed by the child context, something like the following:

func (l profileStatements) ConsumeProfiles(ctx context.Context, ld pprofile.Profiles) error {
	for _, rprofile := range ld.ResourceProfiles().All() {
		for _, sprofile := range rprofile.ScopeProfiles().All() {
			for _, profile := range sprofile.Profiles().All() {
				for _, sample := range profile.Sample().All() {
					for i := sample.LocationsStartIndex(); i < sample.LocationsLength(); i++ {
						location := tCtx.GetProfilesDictionary().LocationTable().At(int(i))
						tCtx := ottlprofilelocation.NewTransformContext(
							profile,
							sample,
							location,
							tCtx.GetProfilesDictionary(),
							sprofile.Scope(),
							rprofile.Resource(),
							sprofile,
							rprofile,
						)
						...
					}
				}
			}
		}
	}
}

Another question we need to answer is: should the transform processor change the pprofile.ProfilesDictionary location entry? and consequently apply the changes to all other data that's referring that same location? or should it work similarly to the profile's attributes, which a copy of the key is created, and indexes are updated to reference the new value?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback 🙏 I appreciate that!

Abstracting the proto details for a moment, if the logical hierarchy is profile -> sample -> location (indepently of where location is stored), then we're dealing with the same span and spanevent pattern, but with a deeper level.

Thinking further about the rest of the submessages, this will become a bit of a burden - like profile -> sample -> location -> line -> function. And I'm a bit worried, how this is supposed to scale on the TransformContext side.

should the transform processor change the pprofile.ProfilesDictionary location entry? and consequently apply the changes to all other data that's referring that same location? or should it work similarly to the profile's attributes, which a copy of the key is created, and indexes are updated to reference the new value?

For consistency, I suggest, for all lookup tables in pprofile.ProfilesDictionary like mappings, locations, functions, links and strings, to keep the same mechanics as for attributes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will set this change to draft for the moment, to wait for #42107 to land first, to reduce duplicate code, as well to address the mentioned feedback.

Co-authored-by: Edmo Vamerlatti Costa <11836452+edmocosta@users.noreply.github.com>
@florianl florianl marked this pull request as draft August 22, 2025 13:39
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 6, 2025
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Sep 20, 2025
@edmocosta edmocosta reopened this Sep 23, 2025
@github-actions github-actions bot removed the Stale label Sep 24, 2025
edmocosta added a commit that referenced this pull request Sep 30, 2025
…2107)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Most Profiling messages do have some attributes. Create ctxprofilecommon
for shared functionality.

Follow up to
#41814 (comment)


<!-- Issue number (e.g. #1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Fixes

<!--Describe what testing was performed and which tests were added.-->
#### Testing

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

---------

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Co-authored-by: edmocosta <11836452+edmocosta@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 8, 2025
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Oct 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[pkg/ottl] Add support for profiles “Location” sub-type

4 participants